Skip to content

[zk] Modified sc_tags in zk.py to include user-specified tags.#3078

Merged
truthbk merged 1 commit intoDataDog:masterfrom
arzarif:zk-status-check-tags
Jan 30, 2017
Merged

[zk] Modified sc_tags in zk.py to include user-specified tags.#3078
truthbk merged 1 commit intoDataDog:masterfrom
arzarif:zk-status-check-tags

Conversation

@arzarif
Copy link
Contributor

@arzarif arzarif commented Dec 13, 2016

What does this PR do?

The manner in which tags are applied to service checks and metric checks depends on 2 separate variables. This change modifies the variable that encapsulates tags relevant to service checks (sc_tags). Currently for service checks, this variable applies only the host and port information provided in the instance configuration. This change will allow user-defined tags to similarly be applied to service checks (as is the case with metric checks).

Motivation

Several of the core integrations handle tags differently for the purposes of metric checks vs. service checks. It's unclear what motivated the inconsistent handling of tags in these contexts. When users are using tags to template alerts, this presents an unnecessary limitation that impacts the ability to template service check-based alerts.

While this PR addresses this inconsistency in one of these integrations (Zookeeper), there should be an effort to normalize the handling of tags in different contexts across natively-supported integrations.

@hkaj hkaj added this to the 5.11.0 milestone Jan 2, 2017
@hkaj hkaj self-requested a review January 2, 2017 15:02
Copy link
Member

@hkaj hkaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, thanks @arzarif

checks.d/zk.py Outdated
tags = instance.get('tags', [])
cx_args = (host, port, timeout)
sc_tags = ["host:{0}".format(host), "port:{0}".format(port)]
sc_tags = ["host:{0}".format(host), "port:{0}".format(port)] + tags
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually a nitpick here: could you list(set(...)) that please? Just in case...

@hkaj hkaj added the community label Jan 2, 2017
@arzarif arzarif force-pushed the zk-status-check-tags branch from 5da094b to 42ce1c8 Compare January 5, 2017 18:38
@masci masci modified the milestones: 5.11.0, 5.12.0 Jan 24, 2017
@truthbk
Copy link
Member

truthbk commented Jan 30, 2017

Thanks @arzarif, very much appreciated.

@truthbk truthbk merged commit a5987e5 into DataDog:master Jan 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants